Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle navigation and traversal cancelation more uniformly #143

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Aug 4, 2021

Previously, cancelation of queued-up traversals was ignored. Now it triggers the appropriate finalization algorithm.

The introduction of a "fired navigate event" boolean on the ongoing app history API navigation struct also ensures that we don't fire navigateerror on cases where we never fire the navigate event.

Also closes #141 while we're there.

Finally, this removes the microtask delay in "finalize with an aborted navigation error". This works poorly in practice, because if a navigation interrupts another navigation, you want the navigateerror for the old navigation to fire before the navigate for the new navigation.


Preview | Diff

@domenic domenic requested a review from natechapin August 4, 2021 22:53
Previously, cancelation of queued-up traversals was ignored. Now it triggers the appropriate finalization algorithm.

The introduction of a "fired navigate event" boolean on the ongoing app history API navigation struct also ensures that we don't fire navigateerror on cases where we never fire the navigate event.

Also closes #141 while we're there.
@domenic
Copy link
Collaborator Author

domenic commented Aug 4, 2021

Some analysis of whether this "finalizes with an aborted navigation error" often enough. In particular we need to think about the cases listed in whatwg/html#6927 when the SDT or SDN is extended by respondWith().

Example:

appHistory.addEventListener("navigate", e => {
  e.respondWith(new Promise(r => setTimeout(r, 1_000)));
});

await appHistory.navigate("#foo");

// What does this do?
const p1 = appHistory.navigate("#bar");
history.back();

This is a SDN-SDT case, which in the pre-app history world is straightforward: the SDN completes synchronously, and so the SDT then proceeds. (We would end up on #foo.)

In the app history world however, at the time history.back() is called, the navigation to #bar is still "ongoing". That is, location.href is updated to #bar already, but the loading spinner is still spinning, and navigatesuccess hasn't fired, and p1 hasn't settled.

So we have to decide what it means to traverse back while a navigation is still ongoing. IMO the simplest is that the navigation counts as aborted: it didn't reach the end state, as signaled by the developer's promise passed to respondWith().

Indeed, in this case we match the behavior for cross-document navs/traversals: per whatwg/html#6927, XDN + XDT results in the XDN being canceled. So in this case developers could have the intuition "using respondWith() to extend a same-document nav/traversal makes their overlap behave like the corresponding cross-document case".

The app history spec draft, after this PR, gives the desired result. Because of the clause saying "whenever you would normally cancel non-matured cross-document navigations, also inform app history about canceling navigation", we properly reject p1 and fire navigateerror.


Let's consider the reverse case:

appHistory.addEventListener("navigate", e => {
  e.respondWith(new Promise(r => setTimeout(r, 1_000)));
});

await appHistory.navigate("#foo");

// What does this do?
const p1 = appHistory.back();
const p2 = appHistory.navigate("#bar");

This is a SDT-SDN case, which does not have interoperable behavior: Firefox queues up the SDT to happen after the SDN, whereas Chrome ignores the SDN. It's not clear what direction the spec will go in yet, but I would say that if the SDN is ignored, then p2 should reject (and no navigate event should fire for it).

If you look at the corresponding XDT-XDN case, trying to port over our intuition from above, you find from whatwg/html#6927 that the XDN is ignored in both Chrome and Firefox. So that indicates maybe Chrome's behavior is a bit more uniform.

The app history spec draft, after this PR, does not reject p2 in such a situation. App history is only informed when the browser cancels navigations, not when it ignores them. Hmm.

@domenic domenic merged commit 0e75539 into main Aug 6, 2021
@domenic domenic deleted the cancel-more branch August 6, 2021 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Early bail out spec for canRespond = false, cancelable = false seems wrong
2 participants